Skip to content

Implements missing Header.asMap to return a Map #16340

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

Implements missing Header.asMap to return a Map #16340

wants to merge 1 commit into from
+11 −0

Conversation

ankianan
Copy link

@ankianan ankianan commented Sep 21, 2025 •
edited by qodo-merge-pro bot
Loading

User description

🔗 Related Issues

Fixes #16339

💥 What does this PR do?

Implement asMap function for Header class

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Bug fix (backwards compatible)

PR Type

Enhancement

Description

  • Add asMap() method to Header class in BiDi network types

  • Converts Header object to Map with name and value entries

  • Implements missing functionality for Header serialization

Diagram Walkthrough

flowchart LR
  A["Header class"] --> B["asMap() method"]
  B --> C["Map with name/value"]
  C --> D["Serialized Header data"]
Loading

File Walkthrough
Relevant files
Enhancement
networkTypes.js
Add Header asMap method                                                                   

javascript/selenium-webdriver/bidi/networkTypes.js

  • Add asMap() method to Header class
  • Returns Map with "name" and "value" entries
  • Value is converted using nested asMap() call
 +11/-0   

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@selenium-ci selenium-ci added C-nodejs JavaScript Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels Sep 21, 2025
@qodo-merge-pro Qodo Merge Pro
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

API Consistency

Confirm that returning a native Map from Header.asMap() aligns with surrounding BiDi type serializers; other types may return plain objects. Inconsistent return types could complicate consumers and serialization.

 
asMap() {
      return new Map([
          ["name", this.name],
          ["value", Object.fromEntries(this.value.asMap())]
      ]);
  };
Value Handling

Object.fromEntries(this.value.asMap()) assumes value.asMap() returns entries; verify BytesValue.asMap() exists and matches expected structure. If value can be null/primitive, add guards to prevent runtime errors.

 
    ["name", this.name],
    ["value", Object.fromEntries(this.value.asMap())]
]);
Trailing Semicolon/Method Syntax

The class method ends with }; which is unnecessary for class method definitions and may violate lint rules; consider removing the trailing semicolon.

 
asMap() {
      return new Map([
          ["name", this.name],
          ["value", Object.fromEntries(this.value.asMap())]
      ]);
  };

@qodo-merge-pro Qodo Merge Pro
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Ensure consistent nested Map structures

In the Header.asMap method, remove the Object.fromEntries call to ensure the
nested value remains a Map, maintaining data structure consistency.

javascript/selenium-webdriver/bidi/networkTypes.js [128-131]

 return new Map([
     ["name", this.name],
-    ["value", Object.fromEntries(this.value.asMap())]
+    ["value", this.value.asMap()]
 ]);
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that converting the nested Map from this.value.asMap() to an object using Object.fromEntries creates an inconsistent data structure, and the proposed change improves API consistency.

Low
  • More

@ankianan ankianan changed the title Update networkTypes.js Implements missing Header.asMap to return a Map Sep 21, 2025
Delta456
Delta456 reviewed Sep 21, 2025
View reviewed changes
Copy link
Member

@Delta456 Delta456 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test for this?

@cgoldberg
Copy link
Member

@ankianan The linter is failing on this.
Can you run ./scripts/format.sh and then update your branch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Delta456 Delta456 Delta456 left review comments

Assignees
No one assigned
Labels
B-devtools Includes everything BiDi or Chrome DevTools related C-nodejs JavaScript Bindings Review effort 2/5
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: TypeError: header.asMap is not a function
5 participants
@ankianan @CLAassistant @cgoldberg @Delta456 @selenium-ci